pgsql-hackers
❮
[Refactor]Avoid to handle FORCE_NOT_NULL/FORCE_NULL options when COPY TO
- Jump to comment-1zmlpostgres@gmail.com2022-07-26T16:16:33+00:00Hi, FORCE_NOT_NULL and FORCE_NULL are only used when COPY FROM. And copyto.c and copyfrom.c are split in this commit https://github.com/postgres/postgres//commit/c532d15dddff14b01fe9ef1d465013cb8ef186df <https://github.com/postgres/postgres//commit/c532d15dddff14b01fe9ef1d465013cb8ef186df> . There is no need to handle these options when COPY TO. Regards, Zhang Mingli
- Jump to comment-1horikyota.ntt@gmail.com2022-07-27T04:55:49+00:00At Wed, 27 Jul 2022 00:16:33 +0800, Zhang Mingli <zmlpostgres@gmail.com> wrote in > FORCE_NOT_NULL and FORCE_NULL are only used when COPY FROM. > > And copyto.c and copyfrom.c are split in this commit https://github.com/postgres/postgres//commit/c532d15dddff14b01fe9ef1d465013cb8ef186df <https://github.com/postgres/postgres//commit/c532d15dddff14b01fe9ef1d465013cb8ef186df> . > > There is no need to handle these options when COPY TO. Agreed. ProcessCopyOptions previously rejects force_quote_all for COPY FROM and copyfrom.c is not even conscious of the option (that is, even no assertion on it). The two options are rejected for COPY TO by the same function so it seems like a thinko of the commit. Getting rid of the code would be good in the view of code coverage and maintenance. On the otherhand I wonder if it is good that we have assertions on the option values. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
- Jump to comment-1guofenglinux@gmail.com2022-07-27T06:37:40+00:00On Wed, Jul 27, 2022 at 12:55 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > ProcessCopyOptions previously rejects force_quote_all for COPY FROM > and copyfrom.c is not even conscious of the option (that is, even no > assertion on it). The two options are rejected for COPY TO by the same > function so it seems like a thinko of the commit. Getting rid of the > code would be good in the view of code coverage and maintenance. Yeah, ProcessCopyOptions() does have the check for force_notnull and force_null whether it is using COPY FROM and whether it is in CSV mode. So the codes in copyto.c processing force_notnull/force_null are actually dead codes. > On the otherhand I wonder if it is good that we have assertions on the > option values. Agree. Assertions would be better. Thanks Richard
- Jump to comment-1zmlpostgres@gmail.com2022-07-28T13:04:04+00:00Hi, all Assertions added. Thanks for review. Regards, Zhang Mingli Sent with a Spark On Jul 27, 2022, 14:37 +0800, Richard Guo <guofenglinux@gmail.com>, wrote: > > > On Wed, Jul 27, 2022 at 12:55 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > > ProcessCopyOptions previously rejects force_quote_all for COPY FROM > > > and copyfrom.c is not even conscious of the option (that is, even no > > > assertion on it). The two options are rejected for COPY TO by the same > > > function so it seems like a thinko of the commit. Getting rid of the > > > code would be good in the view of code coverage and maintenance. > > > > Yeah, ProcessCopyOptions() does have the check for force_notnull and > > force_null whether it is using COPY FROM and whether it is in CSV mode. > > So the codes in copyto.c processing force_notnull/force_null are > > actually dead codes. > > > > > On the otherhand I wonder if it is good that we have assertions on the > > > option values. > > > > Agree. Assertions would be better. > > > > Thanks > > Richard
- Jump to comment-1guofenglinux@gmail.com2022-07-29T03:23:57+00:00On Thu, Jul 28, 2022 at 9:04 PM Zhang Mingli <zmlpostgres@gmail.com> wrote: > Assertions added. > Can we also add assertions to make sure force_quote, force_notnull and force_null are available only in CSV mode? Thanks Richard
- Jump to comment-1zmlpostgres@gmail.com2022-08-01T01:59:49+00:00HI, More assertions added. Thanks. Regards, Zhang Mingli On Jul 29, 2022, 11:24 +0800, Richard Guo <guofenglinux@gmail.com>, wrote: > > > On Thu, Jul 28, 2022 at 9:04 PM Zhang Mingli <zmlpostgres@gmail.com> wrote: > > > Assertions added. > > > > Can we also add assertions to make sure force_quote, force_notnull and > > force_null are available only in CSV mode? > > > > Thanks > > Richard
- Jump to comment-1horikyota.ntt@gmail.com2022-08-02T04:30:50+00:00At Mon, 1 Aug 2022 09:59:49 +0800, Zhang Mingli <zmlpostgres@gmail.com> wrote in > On Jul 29, 2022, 11:24 +0800, Richard Guo <guofenglinux@gmail.com>, wrote: > > > > > On Thu, Jul 28, 2022 at 9:04 PM Zhang Mingli <zmlpostgres@gmail.com> wrote: > > > > Assertions added. > > > > > > Can we also add assertions to make sure force_quote, force_notnull and > > > force_null are available only in CSV mode? > > More assertions added. An empty List is not NULL but NIL (which values are identical, though). There are some other option combinations that are rejected by ProcessCopyOptions. On the other hand *re*checking all combinations that the function should have rejected is kind of silly. Addition to that, I doubt the assertions are really needed even though the wrong values don't lead to any serious consequence. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
- Jump to comment-1zmlpostgres@gmail.com2022-08-02T08:13:30+00:00Regards, Zhang Mingli On Aug 2, 2022, 12:30 +0800, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, wrote: > An empty List is not NULL but NIL (which values are identical, > though). Thanks for pointing that out. Fix it in new patch. > There are some other option combinations that are rejected > by ProcessCopyOptions. On the other hand *re*checking all > combinations that the function should have rejected is kind of silly. > Addition to that, I doubt the assertions are really needed even though > the wrong values don't lead to any serious consequence. > Agree. ProcessCopyOptions has rejected all invalid combinations and assertions are optional. > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center